fix: harden ldk node teardown on stop - #1100
Conversation
…the handle on the LDK queue instead of leaving it to the GC finalizer
Greptile SummaryThis PR hardens Lightning node shutdown and avoids unnecessary restarts during short background periods. The main changes are:
Confidence Score: 5/5This looks safe to merge.
|
| Filename | Overview |
|---|---|
| app/src/main/java/to/bitkit/repositories/LightningRepo.kt | Adds synchronized deferred-stop handling and protects lifecycle teardown from cancellation. |
| app/src/main/java/to/bitkit/services/LightningService.kt | Releases native node resources deterministically with a bounded wait. |
| app/src/main/java/to/bitkit/viewmodels/WalletViewModel.kt | Cancels deferred stops on foreground entry and debounces background stops. |
| app/src/test/java/to/bitkit/repositories/LightningRepoTest.kt | Adds tests for deferred stopping, cancellation, lifecycle state, and teardown ordering. |
| app/src/test/java/to/bitkit/services/LightningServiceTest.kt | Adds tests for handle release, cancellation, stop failures, and repeated shutdown. |
| app/src/test/java/to/bitkit/ui/WalletViewModelTest.kt | Adds coverage for cancelling a deferred stop while startup is active. |
Reviews (3): Last reviewed commit: "Merge branch 'master' into fix/node-stop..." | Re-trigger Greptile
This comment was marked as outdated.
This comment was marked as outdated.
iOS port assessment: not neededChecked this against the iOS teardown paths (
The one genuinely shared item is the underlying defect, synonymdev/ldk-node#94 — which this PR explicitly does not fix, it only narrows the timing window. That's a Rust-side fix and applies to both platforms whenever it lands upstream; it isn't something to port at the app layer. One minor, non-blocking note for the iOS side: |
ovitrif
left a comment
There was a problem hiding this comment.
I re-audited the teardown and lifecycle paths against the current head and reproduced the two inline races with focused coroutine tests.
ovitrif
left a comment
There was a problem hiding this comment.
I re-audited exact head 2955bd0 after the two earlier P1 fixes. Those cancellation and deferred-stop findings are resolved, and all 18 hosted checks plus the focused LightningServiceTest, LightningRepoTest, and WalletViewModelTest suites pass.
Two distinct high-priority native-lifecycle findings remain inline, so I’m leaving this as a neutral review. I also installed this head on the Android 17 / 16 KB emulator and completed short and long background/foreground smoke cycles without a crash or ANR. The preserved dev wallet’s configured local Electrum endpoint (10.0.2.2:60001) was unavailable, so that device pass did not exercise teardown from a fully running node.
…the LDK event handler, which runs on listenerJob, so listenerJob.cancelAndJoin() waits on itself
|
checking the best approach for the electrum restart taking too long https://github.com/synonymdev/bitkit-android/actions/runs/30292125774/job/90105118405?pr=1100 |
Relates to #982 , #986
Upstream issue: synonymdev/ldk-node#94
This PR hardens LDK node teardown so it cannot orphan, deadlock, or overlap the native node:
free_nodecan never brick startupDescription
Play Vitals reports a
SIGABRTon mainnet as the top production crash cluster. Symbolicating it locally against the unstrippedlibldk_node.sofor the shippedldk-node-androidversion resolved every frame and gave a clear cause:The defect itself is upstream: ldk-node's chain-sync task drops the last reference to the Electrum client, which owns the tokio runtime, while running on a worker thread of that same runtime. Release Rust aborts on panic, so the process dies with no catchable error. That is filed as synonymdev/ldk-node#94 and this PR does not fix it. What this PR fixes is the Android teardown that made the window wide, non-deterministic, and prone to overlapping native lifetimes.
Deterministic, bounded, non-cancellable release
Stopping the node cleared the reference but never destroyed the handle, so the native node stayed alive until the garbage collector freed it on the finalizer thread at an arbitrary later moment, potentially while a new node was already running. The handle is now released as part of the stop itself.
The release is bounded, not unconditionally synchronous.
free_nodereturns in tens of milliseconds for a healthy node but blocks for tens of seconds when a failed start left the node's background tasks wedged (measured: ~38 ms healthy vs ~40 s wedged). So the release runs on the IO dispatcher and the stop waits on it only up to a short timeout — long enough that a healthy release completes inline, but bounded so the lifecycle state (Stopped) is published promptly rather than blocking behind a wedged drain.Teardown was also abandonable. The stop ran through a cancellable context and was triggered from a ViewModel scope, so an activity finishing mid-stop left a live native node orphaned with the lifecycle state stranded. The whole teardown — including the listener cleanup join at the top — is now non-cancellable once committed. Reading ldk-node also showed the stop path could not fail the way the code assumed: it reports an error only when the node is already stopped and swallows every internal failure, so a failed stop no longer rethrows and skips the release.
Listener lifecycle safety
WakeNodeWorkercallsstop()synchronously from its registered LDK event handler, which runs on the listener job.listenerJob.cancelAndJoin()then waited on the very job it was running on — a self-join. The same shape applied tostartEventListener()'s re-arm. A stop from inside a handler now skips joining its own job (detected viacurrentCoroutineContext(), not the shadowing class-scopecoroutineContext), and a re-arm from inside a handler is a no-op. The listener loop also keys on node identity, not just the sharedshouldListenForEventsflag (now@Volatile), so a racingstart()cannot make the old loop poll a nodedestroy()already freed.startEventListenerusesrunSuspendCatchingso it no longer silently swallows the cancellation.Overlap prevention: release gate + non-blocking recovery
Even with a bounded stop, the old node can still be draining after
Stoppedis published. Rebuild,wipeStorage(),resetNetworkGraph(), and the pathfinding-scores VSS deletes now wait on the previous node's release before touching storage, so a new node never builds over — or a delete never races — storage the old node still owns. That wait is itself bounded (~90 s, comfortably above the observed ~40 s wedge): a stuckfree_nodethrowsNodeReleaseTimeoutrather than proceeding (which would overlap) or blocking forever (which would permanently brick every future rebuild).Because the gate deliberately makes a wedged-node rebuild wait for the drain, the Electrum/RGS "wrong server" recovery would otherwise block the user-facing error behind ~40 s. So
restartWithElectrumServer/restartWithRgsServernow surface the failure immediately and run the recovery restart on the repository's process-lifetime scope; the gate still holds inside that background recovery. An earlier revision that waited for the release inline turned a 0.6 s recovery stop into a 40 s one and timed out the@settings_10E2E; this keeps the gate and its overlap guarantee while surfacing the error in ~23 s.Brief-background debounce
Backgrounding the app stopped the node immediately, so a short task switch cost a full stop plus a full rebuild. The stop from backgrounding is now deferred by a few seconds and cancelled if the app returns. Only the backgrounding path is deferred; the notification stop action, service teardown, notification worker, and restore migration all still stop immediately. The deferred stop is owned by the repository's process-lifetime scope so it cannot be silently dropped when the activity goes away, and every
start()cancels a pending stop up front so a start that short-circuits on its guards can't leave one to fire against a foregrounded node.Preview
2-seconds-pause.webm
long-pause-with-channel.webm
QA Notes
Manual Tests
regression:Background Payments → enable Get paid when Bitkit is closed and Keep Bitkit active in background → background the app: node keeps running, no teardown.regression:Node notification → tap Stop: node stops immediately and the app task is removed.regression:Relaunch after a notification stop: node rebuilds and reaches started.regression:Settings → Lightning → restart node, and Electrum/RGS server change to a valid server: node stops and restarts normally.regression:Receive a Lightning payment in the background (app closed, Background Payments on): payment is received and the notification is delivered.Automated Checks
LightningServiceTest.kt: deterministic handle release, release when the node is already stopped / when node stop throws, the no-node case, no double release, and teardown completing when the caller is cancelled (including cancellation during listener cleanup).LightningServiceTest.kt: a stop from inside an event handler completes without self-join deadlock, a re-arm from inside a handler keeps the listener running, an external stop cancels and joins the listener, and the loop stops polling a node once it is swapped out (no use-after-free).LightningServiceTest.kt: rebuild (setup),wipeStorage, andresetNetworkGrapheach wait for the previous release before proceeding; the gate adds no latency when no release is pending; the gate throws instead of blocking forever when the release never finishes; and the stop returns within its bound while the release is wedged. These use a real IO dispatcher plus a controllably delayeddestroy().LightningRepoTest.kt: deferred stop timing and cancellation, a foreground/background cycle within the window never stopping the node, andrestartWithElectrumServersurfacing failure before the background recovery completes.WalletViewModelTest.kt: a foreground start cancels a pending deferred stop even while startup is still active.Stopping node…→Node stopped→Building node…→Node started, confirming teardown completes before startup on the healthy path.@settings_10"wrong Electrum server" condition). The error surfaces in ~23 s (under the 30 s E2E budget) while the gate holds the background rebuild until the wedged node drains, with noSIGABRT/tombstone:Fatal signal,SIGABRTor tombstone appeared in logcat across any of the runs above.